-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(share_plus)!: SharePlus refactor #3404
base: main
Are you sure you want to change the base?
Conversation
One last big piece missing is the Windows refactor, which I plan to do tomorrow or so. Then I'll write up a migration guide on the README.md. The API and functionality is still backwards compatible, but just getting deprecated warnings. The old The
|
I will review this again later tonight and merge if everything looks ok still |
@miquelbeltran |
Hey @StanleyCocos I am very sorry I wanted to have this done during the winter break, and then I got sidetracked. Can I ask you a favor? Could you take a look and give me a review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the share_plus package by unifying sharing methods into a single non-static approach using a singleton instance and the ShareParams class.
- Consolidated sharing methods into a single instance method call.
- Updated examples and documentation in the README.md to reflect the new API usage.
- Added a migration guide for converting from the deprecated static API.
Files not reviewed (19)
- packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/MethodCallHandler.kt: Language not supported
- packages/share_plus/share_plus/android/src/main/kotlin/dev/fluttercommunity/plus/share/Share.kt: Language not supported
- packages/share_plus/share_plus/example/ios/Runner/AppDelegate.swift: Language not supported
- packages/share_plus/share_plus/example/lib/main.dart: Language not supported
- packages/share_plus/share_plus/example/windows/flutter/CMakeLists.txt: Language not supported
- packages/share_plus/share_plus/example/windows/runner/Runner.rc: Language not supported
- packages/share_plus/share_plus/ios/share_plus/Sources/share_plus/FPPSharePlusPlugin.m: Language not supported
- packages/share_plus/share_plus/lib/share_plus.dart: Language not supported
- packages/share_plus/share_plus/lib/src/share_plus_linux.dart: Language not supported
- packages/share_plus/share_plus/lib/src/share_plus_web.dart: Language not supported
- packages/share_plus/share_plus/lib/src/share_plus_windows.dart: Language not supported
- packages/share_plus/share_plus/macos/share_plus/Sources/share_plus/SharePlusMacosPlugin.swift: Language not supported
- packages/share_plus/share_plus/test/share_plus_linux_test.dart: Language not supported
- packages/share_plus/share_plus/test/share_plus_test.dart: Language not supported
- packages/share_plus/share_plus/test/share_plus_windows_test.dart: Language not supported
- packages/share_plus/share_plus/windows/share_plus_plugin.cpp: Language not supported
- packages/share_plus/share_plus/windows/share_plus_windows_plugin.h: Language not supported
- packages/share_plus/share_plus_platform_interface/lib/method_channel/method_channel_share.dart: Language not supported
- packages/share_plus/share_plus_platform_interface/lib/platform_interface/share_plus_platform.dart: Language not supported
Co-authored-by: Copilot <[email protected]>
Fixing the analysis errors, and main is merged in as well. I am going to mark this as a breaking change, although technically it is not, using this package will cause deprecated warnings to appear and the change is big enough that it deserves it. I am setting myself the goal to merge this by April 18th (two weeks from now). Please, @StanleyCocos and anyone else interested, I'd appreciate a lot to have a review of the changes. |
Of course, I’ll try to take a look and offer some suggestions from my side. Things have been quite busy for me recently, but as you mentioned, the merge is in two weeks — I’ll do my best to wrap this up before then. Thanks again! |
throw ArgumentError( | ||
'fileNameOverrides must have the same length as files.', | ||
); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Share.downloadFallbackEnabled is a global parameter, we cannot override the functionality already configured by the user. For example, if the user has set Share.downloadFallbackEnabled = false, but the default in ShareParams is true.
/// Cannot be used in combination with [text]. | ||
/// | ||
/// * Supported platforms: iOS, Android | ||
/// Fallsback to sharing the URI as text on other platforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fallsback to Falls back
|
||
return ShareResult(result, _statusFromResult(result)); | ||
return map; | ||
} | ||
|
||
/// Ensure that a file is readable from the file system. Will create file on-demand under TemporaryDiectory and return the temporary file otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TemporaryDiectory to TemporaryDirectory
files: [XFile(fd.path)], | ||
), | ||
); | ||
verify(mockChannel.invokeMethod<String>('share', <String, dynamic>{ | ||
'paths': [fd.path], | ||
'mimeTypes': ['image/png'], | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add some mock data for ShareResult. Right now, it seems we only see ShareResult.unavailable.
Can we add some test cases specifically for sharing to ensure the platform-side code works correctly? |
Description
This PR contains several improvements on the share_plus package:
SharePlus
class viainstance
singleton, rather than using static methods.SharePlatform
for testing.subject
andtitle
.ShareParams
class.mailToFallbackEnabled
to disable web mailTo failback (enabled by default)downloadFallbackEnabled
is no longer a static setting but part of theShareParams
uri
is now supported on all platforms by sharing the URI as plain textNot a breaking change, since the old API is still compatible, only it has been deprecated.
But we should consider bumping the version of
share_plus
to a major release nevertheless, since the deprecation will break CI/CD lint analysis.TODO:
Related Issues
share_plus
refactor #3403EXTRA_TITLE
in share Intent on Android #3307Checklist
CHANGELOG.md
nor the plugin version inpubspec.yaml
files.flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
!
in the title as explained in Conventional Commits).